fix and simplify build #85
Conversation
|
Thanks. d42fe54 looks good and I've commited as a36c7dc 8a6529a also has some good chunks modernizing CMake usage , but it goes too far in simplifing away important workarounds for GCC/Clang quirks, as can be seen by the failed builds/tests on https://github.com/jrfonseca/drmingw/actions/runs/4713642077 I'll try to get the non-controversial chunks committed, and the remaining will need to be address seperately. |
|
I will try to fix the failing tests. |
|
Thanks. I think the easier is to break the simplifications in smaller independent patches, starting with the uncontroversial chunks first, then slowly push those more suspect (the static linking, rewrite of import libraries) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #85 +/- ##
==========================================
- Coverage 50.62% 50.57% -0.05%
==========================================
Files 15 15
Lines 1997 1993 -4
Branches 758 757 -1
==========================================
- Hits 1011 1008 -3
+ Misses 761 759 -2
- Partials 225 226 +1 ☔ View full report in Codecov by Sentry. |
oltolm
force-pushed
the
cmake
branch
4 times, most recently
from
7e779bb to
0c6af50
Compare
|
So I fixed all tests except for build-cross-clang. I don't know why it fails. |
|
Thanks for the update.
I depend upon Clang to build the Arm binaries, so I can't afford to break the Clang builds. I also must insist upon the simplifications being broken up in smaller independent commits no matter what. Even if you fix all test regressions, the test suite I have is not 100% bulletproof, and these changes touch upon things that gave me lots of headaches in the past, taking many tries to get it working. Consequently I wouldn't be surprised if there were some unintended side effects here somehow that are escaping the test suite. Therefore I want to be able to easily bisect and revert parts of PR in the future, and for that I need it to be broken up in smaller independent changes. I appreciate the attention you put on this so far -- this MR has been very educational for me -- CMake seems to have made quite a few things easier --, but I'm afraid it's not something I can commit as-is. |
|
I got the test to pass. I can try splitting the code into small commits. |
|
Thank you! |
|
So I tried splitting the code into 6 commits. This was more effort than I anticipated. |
|
Thanks for going the extra mile. I'm afraid dependencies with ninja are broken when I manually run Sometimes its I also see This is what I'm going to do: I'm going to go every commit one by one, keep the good bits, remove any troublesome bits. Hopefully this will push things forward without breaking anything. |
|
Sure. I don't see a file |
|
Sorry, I'm not sure why it always fails when locally, but not on GitHub Actions -- perhaps because it's a race condition and I have more CPU threads than GitHub VMs. For me |
|
I merged the a good chunk of the changes. The mgwhelp import lib and test dependencies seem to be more problematic. I'll leave those alone for now. |
oltolm
force-pushed
the
cmake
branch
2 times, most recently
from
5232a5c to
b4a10b3
Compare
|
Hi @oltolm, The build started failing on master due to some weird race condition on the import libs. I tried these changes again, and they do avoid the race condition. However I feared regressions, so I wrote this script to make sure See https://github.com/jrfonseca/drmingw/actions/runs/7151760733/job/19476380076: So your changes cause all EXEs to link against The issue is the PUBLIC in mgwhelp, but the build will fail with missing symbols if this is removed: diff --git a/src/mgwhelp/CMakeLists.txt b/src/mgwhelp/CMakeLists.txt
index f38339e..ea1702f 100644
--- a/src/mgwhelp/CMakeLists.txt
+++ b/src/mgwhelp/CMakeLists.txt
@@ -14,9 +14,8 @@ target_sources (mgwhelp PRIVATE
target_link_libraries (mgwhelp PRIVATE
common
dwarf
libiberty
- PUBLIC
dbghelp
)
set_target_properties (mgwhelp PROPERTIESIn short, I'm afraid that change a07674c is a false simplification: it simplifies the build by removing (the admittedly complex!) workarounds necessary to get this to work... I wished there was a simple way to workaround MinGW 32-bits DLL import library issue, but I don't see a solution besides using separate DEF for imports/exports... |
|
I tried another fix. |
|
You need to revert f9c837e on your branch. |
|
I think it is a bug in ld. I fixed it in https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=d544a1dca23469d212e7e9edc754487352c5f042. |
Awesome! Thank you. I'm not sure when that fix will trickle through prebuild mingw binaries like https://github.com/niXman/mingw-builds-binaries but I'll reopen this PR so I don't forget to revisit this in the future. |
|
Reopening... |
I tried build drmingw with mingw-w64, but it failed because
libwinpthread-1.dllwas not getting linked statically. So I tried fixing the problem and in the course simplified the build system. Nowlibwinpthread-1.dllis getting linked statically.